Skip to content

feat(instrumentation): support nextConfig.instrumentationClientInject#1416

Open
Divkix wants to merge 9 commits into
cloudflare:mainfrom
Divkix:fix/issue-1326-instrumentation-client-inject
Open

feat(instrumentation): support nextConfig.instrumentationClientInject#1416
Divkix wants to merge 9 commits into
cloudflare:mainfrom
Divkix:fix/issue-1326-instrumentation-client-inject

Conversation

@Divkix
Copy link
Copy Markdown
Contributor

@Divkix Divkix commented May 21, 2026

Summary

Adds support for the nextConfig.instrumentationClientInject config option introduced in Next.js. This lets users inject client bootstrap modules (for side effects and an optional onRouterTransitionStart hook) ahead of the user's instrumentation-client file, without modifying source files.

Closes #1326.

Changes

  • New virtual module generatorgenerateInstrumentationClientInjectModule() generates ESM with side-effect imports in array order, then the user's instrumentation-client file last (or vinext/client/empty-module when absent). A single composed onRouterTransitionStart fans out to every module's hook.
  • Config types — added instrumentationClientInject to NextConfig and ResolvedNextConfig with default [].
  • Vite plugin wiring — new vinext:instrumentation-client-inject plugin with resolveId + load hooks. The virtual module string is precomputed once in config() to avoid rebuilding on every HMR cycle.
  • Tests — 7 unit tests covering empty injects, single/multi specifiers, hook ordering, and empty-module fallback.

Verification

  • Pure function verified via inline Node assertions
  • tsc --noEmit clean (exit 0)
  • Follows existing virtual module conventions (\0-prefixed IDs, same as virtual:vinext-* modules)
  • Empty instrumentationClientInject is a transparent passthrough (existing resolve.alias continues to handle the user file or empty-module)

Behavioral parity with Next.js

  • Injected modules run for side effects before hydration in array order
  • User instrumentation-client file runs last
  • Composed onRouterTransitionStart fires in config order across all modules
  • Bare npm names and relative paths are resolved by Vite's resolver naturally

…cloudflare#1326)

Add support for the Next.js instrumentationClientInject config option that
lets users inject client bootstrap modules for side effects and an optional
onRouterTransitionStart hook ahead of the user's instrumentation-client file.

Changes:
- New generateInstrumentationClientInjectModule() generates a virtual ESM
  module with side-effect imports in array order + composed hook fan-out.
- Accept instrumentationClientInject in NextConfig / ResolvedNextConfig.
- New vinext:instrumentation-client-inject Vite plugin with resolveId/load
  hooks; virtual module string is precomputed once in config() for efficiency.
- Unit tests cover empty injects, single/multi specifiers, hook ordering,
  and empty-module fallback when no user file exists.
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 21, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1416

commit: 5ce7226

Divkix added 3 commits May 21, 2026 15:35
The test expected 2-space indent for the generated code, but the
implementation produces 4-space indent within if blocks. Also fixes
oxfmt quote style and line wrapping.
- Replace unnecessary escaped quotes inside template literals to fix
  eslint(no-useless-escape) warnings in instrumentation-client-inject.ts
- Add missing instrumentationClientInject field to makeResolved() test
  helper in next-config.test.ts to fix TS2322 type errors
@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat(instrumentation): support nextConfig.instrumentationClientInject

CI is fully green. The overall approach — precomputing the virtual module once in config() and serving it via a thin resolveId/load pair — is clean and follows the repo's existing virtual-module conventions. I reviewed the Next.js source (next-instrumentation-client-loader.ts, create-compiler-aliases.ts) to verify behavioral parity.

There are a few issues worth addressing before merge, ranging from a behavioral gap to some minor improvements.

Summary of issues

  1. Behavioral gap: clientInjectModule not cleared when config reloadsclientInjectModule is set when instrumentationClientInject.length > 0 but never cleared back to null when the config changes to an empty array (e.g., HMR of next.config). Minor in practice since config changes typically restart the dev server, but worth noting.

  2. Missing input validation on inject specifiers — Empty strings and non-string values in the array could produce broken imports. Next.js validates via z.array(z.string()) in its config schema.

  3. Hook composition diverges slightly from Next.js — Next.js uses mod && mod.onRouterTransitionStart && mod.onRouterTransitionStart(...) (short-circuit AND chain), while vinext uses typeof ... === "function". Functionally equivalent for well-behaved modules, but the typeof check is arguably more defensive.

  4. Generated code exports an untyped function — The composed onRouterTransitionStart(url, type) has no type annotations. Not a runtime concern since this is generated code consumed by Vite's pipeline, but worth noting for consistency with the ClientInstrumentationHooks type.

Overall this is a solid first implementation. The issues above are minor enough that they could be addressed in a follow-up.

// nothing that could shadow what the alias resolves.
if (injects.length === 0) {
return "export {};";
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When injects is empty, this returns "export {};" regardless of whether the user has an instrumentation-client file. The PR description says this is intentional ("the alias already handles the user file or empty-module"), which is correct — the resolve.alias at index.ts:1261 maps private-next-instrumentation-client to the user file or empty-module and takes effect when this plugin returns null from resolveId.

However, this means there's an asymmetry in the hook composition path: when injects are present, the composed onRouterTransitionStart fans out to all modules including the user file. When injects are empty, the user's onRouterTransitionStart flows directly through the alias. Both paths work, but it's worth documenting this explicitly in the function's JSDoc (the current doc hints at it but doesn't state the two resolution paths clearly).

Comment thread packages/vinext/src/index.ts Outdated
if (nextConfig.instrumentationClientInject.length > 0) {
clientInjectModule = generateInstrumentationClientInjectModule(
nextConfig.instrumentationClientInject,
instrumentationClientPath,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module string is only computed when instrumentationClientInject.length > 0, but clientInjectModule is never reset to null on subsequent config re-evaluations. In Vite's multi-environment build pipeline, config() can fire multiple times. If nextConfig is already set (the if (!nextConfig) guard at line 1042), this code path is still reached because instrumentationPath/instrumentationClientPath/middlewarePath are intentionally re-evaluated outside that guard.

This is fine today because nextConfig is only resolved once (the guard), so instrumentationClientInject won't change across calls. But if the guard is ever removed (e.g., to support config HMR), clientInjectModule would need an explicit reset:

Suggested change
instrumentationClientPath,
clientInjectModule = nextConfig.instrumentationClientInject.length > 0
? generateInstrumentationClientInjectModule(
nextConfig.instrumentationClientInject,
instrumentationClientPath,
)
: null;

This makes the assignment unconditional and self-clearing, which is safer against future refactors.

trailingSlash: config.trailingSlash ?? false,
output: output === "export" || output === "standalone" ? output : "",
pageExtensions,
instrumentationClientInject: Array.isArray(config.instrumentationClientInject)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this validates that instrumentationClientInject is an array, but doesn't validate that each element is a string. Next.js uses Zod (z.array(z.string())) in config-schema.ts to reject non-string elements. Consider filtering:

Suggested change
instrumentationClientInject: Array.isArray(config.instrumentationClientInject)
instrumentationClientInject: Array.isArray(config.instrumentationClientInject)
? (config.instrumentationClientInject as unknown[]).filter(
(x): x is string => typeof x === 'string'
)
: [],

This is consistent with how optimizePackageImports is handled a few lines above (line 1048).

}

lines.push("");
lines.push("export function onRouterTransitionStart(url, type) {");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the generated function signature has no type annotations. This is fine since it's a virtual module consumed by Vite (not user-facing code), and Vite will treat it as JS regardless. But for consistency with ClientInstrumentationHooks, you could annotate the parameters in the generated code:

export function onRouterTransitionStart(url: string, type: string) {

Not blocking — just a polish suggestion.

);
expect(code).toBe("export {};");
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test coverage for the pure code-generation function. One edge case worth adding: what happens when an inject specifier contains characters that could break the generated import (e.g., a specifier with quotes or backslashes)? JSON.stringify handles this correctly, but a test would document the contract:

it("escapes special characters in specifier paths", () => {
  const code = generateInstrumentationClientInjectModule(
    ['./path/with"quote.js'],
    null,
  );
  expect(code).toContain('from "./path/with\\"quote.js"');
});

Also, there's no integration-level test that verifies the Vite plugin wiring end-to-end (i.e., that the virtual module is actually served when instrumentationClientInject is configured). The unit tests cover generateInstrumentationClientInjectModule in isolation, which is great, but a fixture-based test would catch issues like the resolve.alias vs resolveId ordering concern. This could be a follow-up.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 22, 2026

Review submitted successfully. Here's a summary of the feedback I posted:

PR #1416 adds support for nextConfig.instrumentationClientInject, a Next.js config option that lets users inject client bootstrap modules ahead of the user's instrumentation-client file. The implementation is clean — it precomputes a virtual ESM module at config time and serves it via a resolveId/load plugin pair.

All CI checks pass. I left 5 inline comments covering:

  1. clientInjectModule self-clearing (index.ts:1068) — the conditional assignment should be made unconditional to be safe against future refactors that might remove the nextConfig once-only guard.
  2. Input validation (next-config.ts:1141) — individual array elements aren't validated as strings; suggested filtering to match how optimizePackageImports is handled.
  3. Documentation of dual resolution paths (instrumentation-client-inject.ts:26) — the empty-injects path vs non-empty path have different resolution mechanics worth documenting.
  4. Untyped generated function (instrumentation-client-inject.ts:47) — minor polish suggestion for type annotations.
  5. Missing integration test and edge case (instrumentation.test.ts:397) — suggested a special-character escaping test and noted the absence of an end-to-end Vite plugin wiring test.

github run

Make clientInjectModule self-clearing on config re-eval, filter non-string inject specifiers, document dual resolution paths, and add escaping coverage.

Signed-off-by: Divanshu Chauhan <divkix@divkix.me>
@Divkix Divkix force-pushed the fix/issue-1326-instrumentation-client-inject branch from e9c0784 to 2b44231 Compare May 22, 2026 11:22
@james-elicx
Copy link
Copy Markdown
Member

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: feat(instrumentation): support nextConfig.instrumentationClientInject

The previous review's feedback has been addressed — input validation filters non-strings, the JSDoc documents the dual resolution paths, and the special-character escaping test was added. The clientInjectModule assignment was already self-clearing via the ternary (I think the original comment misread the code slightly; the assignment is unconditional since it's outside the if (!nextConfig) guard).

I did a deeper pass comparing against the Next.js reference implementation (next-instrumentation-client-loader.ts) and the Next.js E2E tests (test/e2e/instrumentation-client-hook/). The implementation is correct and the plugin wiring is sound — enforce: "pre" ensures the resolveId hook runs before vite:resolve applies the resolve.alias fallback, which is the right ordering.

Two remaining items worth discussing, neither blocking:

  1. ESM import ordering vs CJS require ordering — Next.js generates require() calls (CJS), which execute synchronously in source order. This PR generates static import * as ... statements (ESM). The ES module spec evaluates same-level static imports in source order, and Vite/Rolldown preserve this, so in practice the side-effect ordering is correct. But it's a semantic difference worth being aware of if a future edge case surfaces around circular dependencies or async module evaluation.

  2. Missing integration test — The unit tests cover the code-generation function thoroughly. There's no integration test that verifies the virtual module is actually served through Vite's plugin pipeline when instrumentationClientInject is configured. The previous review noted this and suggested it as a follow-up, which seems reasonable.

LGTM with the two inline comments below.

const lines: string[] = [];

for (let i = 0; i < injects.length; i++) {
lines.push(`import * as __vinj_${i} from ${JSON.stringify(injects[i])};`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next.js resolves inject specifiers against the project root before emitting require() calls (via this.resolve(rootContext, spec) in the webpack loader). Here, bare specifiers and relative paths are left for Vite's resolver to handle at import resolution time, which should work correctly since the virtual module's importer context is the project root. Just flagging the difference — if a user reports that relative paths in instrumentationClientInject resolve against the wrong directory, this is the place to look.

}

lines.push("");
lines.push("export function onRouterTransitionStart(url: string, type: string) {");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit from the previous review that's still applicable — the generated function has TypeScript type annotations (url: string, type: string) but this is loaded as a virtual module via the load hook, which Vite treats as JavaScript by default (no id.endsWith('.ts') to trigger the TS transform). This works today because Vite 8 / Rolldown processes all modules through OXC regardless, but if you wanted to be explicit you could either:

  1. Drop the TS annotations from the generated code (make it plain JS), or
  2. Return { code, map: null } with an explicit lang: 'ts' or append .ts to the resolved ID

Not blocking — the current behavior is correct under Vite 8.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 22, 2026

Review submitted. Here's a summary of the feedback:

PR #1416 adds support for nextConfig.instrumentationClientInject, reimplementing the Next.js feature that lets users inject client bootstrap modules ahead of instrumentation-client. The previous review's feedback has been addressed in the latest commit.

I verified against the Next.js reference implementation (next-instrumentation-client-loader.ts) and E2E tests. The implementation is correct:

  • The enforce: "pre" plugin correctly intercepts private-next-instrumentation-client before vite:resolve applies the resolve.alias fallback
  • The clientInjectModule assignment is properly self-clearing (it's outside the if (!nextConfig) guard)
  • Input validation filters non-string array elements
  • The dual resolution paths are well-documented in the JSDoc

I left 2 non-blocking inline comments:

  1. Specifier resolution context — Next.js pre-resolves inject specifiers against the project root via webpack's this.resolve(). This PR leaves them for Vite's resolver, which should work but is a difference to watch if users report relative path issues.

  2. TypeScript annotations in generated JS — The generated virtual module contains TS type annotations (url: string, type: string) but is served via load() without an explicit .ts extension. Works under Vite 8/OXC but could be made explicit.

Overall: LGTM. The integration test gap (no fixture-based test for the full plugin pipeline) is the most notable missing piece, but fine as a follow-up.

github run

Divkix added 3 commits May 22, 2026 10:58
…inject resolution

Add client-environment tests for instrumentationClientInject virtual module
wiring, omit the alias when injects are active so resolveId/load run, reload
next.config when root changes, and pre-resolve relative inject paths. Share
empty-module path via INSTRUMENTATION_CLIENT_EMPTY_MODULE and simplify tests.
CI E2E builds failed because INSTRUMENTATION_CLIENT_EMPTY_MODULE pointed
at empty-module.ts, which is not shipped in dist. Prefer .js when present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support nextConfig.instrumentationClientInject for pre-hydration client bootstrap modules

2 participants